NOICEEE 💪

Reviewable status: 0 of 8 files reviewed, 15 unresolved discussions (waiting on @ArniStarkware and @dafnamatsry)

crates/gateway/src/lib.rs line 3 at r1 (raw file):

// TODO(Arni, 02/04/2024): Make the file structure in this repo consistent.
// Folders.
pub mod compiler;

I think this should be a separate crate, it isn't really related to the gateway.

In fact, if this works out alright, perhaps the compiler might be interested in taking charge of it and importing it there.

crates/gateway/src/compiler/compile.rs line 6 at r1 (raw file):

use thiserror::Error;

// TODO(Arni, 1/05/2024): Remove the dependancy on anyhow. Run in thread.

Already using thread 🤔
WDYM?

Code quote:

// TODO(Arni, 1/05/2024): Remove the dependancy on anyhow. Run in thread.

crates/gateway/src/compiler/compile.rs line 52 at r1 (raw file):

}

fn starknet_sierra_compile(

Consider moving down.

We haven't talked code conventions yet, but in the blockifier we used the rust-analyzer recommendation of ordering by importance, and in particular, public before private.

Yr call until we decide on a standard though.

Code quote:

fn starknet_sierra_compile(

crates/gateway/src/compiler/compile.rs line 61 at r1 (raw file):

        add_pythonic_hints,
        max_bytecode_size,
    } = compilation_args;

Maybe use them from the instance?
Not sure at all, whichever looks nicer, yr call

Code quote:

    let SierraToCasmCompliationArgs {
        allowed_libfuncs_list_name,
        allowed_libfuncs_list_file,
        add_pythonic_hints,
        max_bytecode_size,
    } = compilation_args;

crates/gateway/src/compiler/compile.rs line 72 at r1 (raw file):

    } = serde_json::from_str(
        &fs::read_to_string(file)
            .map_err(|_| CompilationUtilError::ReadInputError { file: file.into() })?,

Will this require writing the file to disk before calling the function?
Can we get around it by passing the program itself as an argument? or will this break a core assumption of the compiler and cause trouble?

Code quote:

        &fs::read_to_string(file)
            .map_err(|_| CompilationUtilError::ReadInputError { file: file.into() })?,

crates/gateway/src/compiler/compile.rs line 74 at r1 (raw file):

            .map_err(|_| CompilationUtilError::ReadInputError { file: file.into() })?,
    )
    .map_err(|_| CompilationUtilError::DeserializationError)?;

Maybe break into two statements, this is just a bit hard to follow

Code quote:

    let ContractClassIgnoreAbi {
        sierra_program,
        sierra_program_debug_info,
        contract_class_version,
        entry_points_by_type,
        _abi,
    } = serde_json::from_str(
        &fs::read_to_string(file)
            .map_err(|_| CompilationUtilError::ReadInputError { file: file.into() })?,
    )
    .map_err(|_| CompilationUtilError::DeserializationError)?;

crates/gateway/src/compiler/compile.rs line 101 at r1 (raw file):

    };

    let sierra_path_clone = sierra_path.to_string(); // Clone sierra_path

arg name is good

Suggestion:

   let sierra_path_clone = sierra_path.to_string();

crates/gateway/src/compiler/compile.rs line 103 at r1 (raw file):

    let sierra_path_clone = sierra_path.to_string(); // Clone sierra_path
    let handle = thread::spawn(move || {
        catch_unwind(AssertUnwindSafe(move || {

Are you sure we have to use this?
I think it is generally recommended to avoid using it directly (paragraph 4 here)

IIUC rust handles catching the panics out of the box.
At least, I know tokio does this out of the box for spawn_blocking.

Code quote:

   catch_unwind(AssertUnwindSafe(move || {

crates/gateway/src/compiler/compile.rs line 108 at r1 (raw file):

    });

    let result = handle.join().expect("Failed to join thread");

What are the possible errors here?

Can you add a test for a panicy compilation?
I wanna be sure (and make it evident for readers) that the panic is indeed caught in the result below, and not in the result that join returns.

Code quote:

    let result = handle.join().expect("Failed to join thread");

crates/gateway/src/compiler/compile.rs line 108 at r1 (raw file):

    });

    let result = handle.join().expect("Failed to join thread");

Also:

1. add tokio to cargo.toml :P
2. add async to the function sig
3. make the test #[tokio::test]

Also plz add a TODO(task_executor) so we'll replace it by calling the (soon to be merged hopefully 🥺 ) task executor.

Rationale: the task executor will use this tokio method for this, so this will behave more similar to it.
(Not using rayon for CPU just yet, we're keeping things simple since we also have IO to think about so let tokio deal with it all.)

spawn_blocking offloads the task to a threadpool, as opposed to std threads, which just run it with impunity (which can starve out other stuff like IO).

BTW, RE the std::thread implementation, I'm not sure the unwind things was necessary even with std threads; I think the panic would just have been caught at join.expect(), as opposed to the match below, but I'm not sure.

Suggestion:

    let result = tokio::task::spawn_blocking(move || {
        starknet_sierra_compile(compilation_args, &sierra_path_clone)    
    }).await;

crates/gateway/src/compiler/compile.rs line 114 at r1 (raw file):

            // A panic here might be a feature.
            panic!("Compilation panicked: {:?}", e)
        }

Add a test for this plz, I don’t quite believe in that slick new panic-catching gizmo just yet, y’all.

Code quote:

        Err(e) => {
            // A panic here might be a feature.
            panic!("Compilation panicked: {:?}", e)
        }

crates/gateway/src/compiler/compile.rs line 114 at r1 (raw file):

            // A panic here might be a feature.
            panic!("Compilation panicked: {:?}", e)
        }

I think we can do this if we know that the compilation panic is due to faulty input from the user (and not due to some bug on our end), is that the case?

If we can't, you may also consider let-else pattern to reduce nesting, I think it might fit nicely here.

Suggestion:

let compilation_result = result?;

crates/gateway/src/compiler/compile.rs line 123 at r1 (raw file):

            }
            CompilationUtilError::StarknetSierraCompilationError(_) => {
                unimplemented!("The user writes a contract that fails to compile to Starknet casm")

unimplemented --> wont fix
todo --> will fix.

Ref: Third paragraph from the top here

Suggestion:

                todo!("The user provids a badly formatted Sierra file.")
            }
            CompilationUtilError::AllowedLibfuncsError(_) => {
                todo!("The user writes a contract using libfuncs that are not allowed.")
            }
            CompilationUtilError::StarknetSierraCompilationError(_) => {
                todo!("The user writes a contract that fails to compile to Starknet casm")

crates/gateway/src/compiler/compile.rs line 130 at r1 (raw file):

            }
        },
        Ok(Ok(casm_contract)) => serde_json::to_string_pretty(&casm_contract)

This is for nesting?

Also, is it necessary for the actual flow? or just for debugging?
Asking cause this is a relatively expensive op for big contracts.

Code quote:

serde_json::to_string_pretty

crates/gateway/test/fixtures/compiler/account_faulty.sierra.json line 2 at r1 (raw file):

{
  "sierra_program": [

Should be in gateway/tests/fixtures/
Don't need compiler namespacing I think, might be useful for others.
testsis the canonical location for this I think

Reviewable status: 0 of 8 files reviewed, 16 unresolved discussions (waiting on @ArniStarkware and @dafnamatsry)

crates/gateway/src/compiler/compile.rs line 52 at r1 (raw file):

removed block 😅

crates/gateway/src/compiler/compile.rs line 61 at r1 (raw file):

removed block 😅

crates/gateway/src/compiler/compile.rs line 74 at r1 (raw file):

removed block 😅

crates/gateway/src/compiler/compile_test.rs line 7 at r1 (raw file):

    let sierra_path = "test/fixtures/compiler/account_faulty.sierra.json";
    let casm = compile_sierra_to_casm(sierra_path);
    assert_eq!(casm.len(), 72304);

Will this length change in between compiler releases?
We don't this test to break whenever we bump a version unless we have to.

Also plz extract to a variable expected_casm_program_len.

Code quote:

    assert_eq!(casm.len(), 72304);

Reviewable status: 0 of 13 files reviewed, 14 unresolved discussions (waiting on @dafnamatsry and @giladchase)

crates/gateway/src/lib.rs line 3 at r1 (raw file):

Done.

About Asking the compilers to take charge of this part: Sounds great and more sound. The biggest hurdle is error handling as I see it.

crates/gateway/src/compiler/compile.rs line 6 at r1 (raw file):

Removed.

crates/gateway/src/compiler/compile.rs line 52 at r1 (raw file):

Done.

crates/gateway/src/compiler/compile.rs line 61 at r1 (raw file):

Done.

crates/gateway/src/compiler/compile.rs line 72 at r1 (raw file):

I think we can get around this issue.

I moved this part of the code to a test util. I redefined the function so that it works on a ContractClass. We can later see how we want to use it in practice.

crates/gateway/src/compiler/compile.rs line 74 at r1 (raw file):

Moved to another file.
I aimed to move this usage to a test util for this PR. Maybe deal with this issue as a feature in future PR if needed.

crates/gateway/src/compiler/compile.rs line 101 at r1 (raw file):

Done.

crates/gateway/src/compiler/compile.rs line 123 at r1 (raw file):

Done.

crates/gateway/src/compiler/compile_test.rs line 7 at r1 (raw file):

IIUC - the length should not change between compiler releases. This is part of the backward compatibility of compilation to casm.

extract to a variable: Done.

crates/gateway/test/fixtures/compiler/account_faulty.sierra.json line 2 at r1 (raw file):

Done.
Moved with the rest of the crate.

Reviewable status: 0 of 13 files reviewed, 14 unresolved discussions (waiting on @dafnamatsry and @giladchase)

crates/gateway/src/compiler/compile.rs line 103 at r1 (raw file):

Now using tokio.

crates/gateway/src/compiler/compile.rs line 108 at r1 (raw file):

Done.

crates/gateway/src/compiler/compile.rs line 108 at r1 (raw file):

Post-change - the only possible error is a JoinError: see.

crates/gateway/src/compiler/compile.rs line 114 at r1 (raw file):

Not sure I understand. Anyway - the original line of code was changed.

crates/gateway/src/compiler/compile.rs line 114 at r1 (raw file):

Added a negative flow test.

Did not add a JoinResult is panic! test yet. Added a TODO, We should start a new thread in a more appropriate location.

crates/gateway/src/compiler/compile.rs line 130 at r1 (raw file):

Now not a part of the actual flow.

Reviewed 1 of 13 files at r2, 2 of 4 files at r6, all commit messages.
Reviewable status: 3 of 13 files reviewed, all discussions resolved (waiting on @dafnamatsry)

crates/starknet_sierra_compile/Cargo.toml line 2 at r6 (raw file):

[package]
name = "starknet_sierra_compile"

Note: Naming might be confusing, as it is too similar to cairo-lang-sierra, but we aren't publishing this crate anytime soon, let's worry about it later :)

Code quote:

name = "starknet_sierra_compile"

crates/starknet_sierra_compile/src/lib.rs line 1 at r6 (raw file):

pub mod compile;

Maybe add tiny module-docstring here:

//! An experimental attempt to compile Sierra into Casm by using cairo-lang as a lib .

crates/gateway/src/lib.rs line 3 at r1 (raw file):

Agreed. The thing about the compiler was meant as a "some-day 🥺" kind of thing , so no need to worry about it

crates/gateway/src/compiler/compile.rs line 103 at r1 (raw file):

🍣

Reviewed 6 of 13 files at r2, 2 of 6 files at r3, 2 of 4 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry)

Reviewable status: 10 of 13 files reviewed, all discussions resolved (waiting on @dafnamatsry and @giladchase)

crates/starknet_sierra_compile/src/lib.rs line 1 at r6 (raw file):

Done.

Reviewed 3 of 3 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry)

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry)

Please add @orizi

Reviewed 6 of 13 files at r2, 2 of 6 files at r3, 2 of 4 files at r6, 1 of 3 files at r7, 2 of 2 files at r8, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @ArniStarkware)

crates/starknet_sierra_compile/src/compile.rs line 13 at r8 (raw file):

pub mod compile_test;

pub struct SierraToCasmCompliationArgs {

Suggestion:

SierraToCasmCompilationArgs

crates/starknet_sierra_compile/src/compile.rs line 15 at r8 (raw file):

pub struct SierraToCasmCompliationArgs {
    allowed_libfuncs_list_name: Option<String>,
    allowed_libfuncs_list_file: Option<String>,

IIUC only one should be supplied.
In that case, please make it an enum.

Also, which one are we going to use? Maybe we should only allow one of them...

Code quote:

    allowed_libfuncs_list_name: Option<String>,
    allowed_libfuncs_list_file: Option<String>,

crates/starknet_sierra_compile/src/compile.rs line 43 at r8 (raw file):

    // TODO(task_executor).
    tokio::task::spawn_blocking(move || starknet_sierra_compile(compilation_args, contract_class))

I think managing the compilation inside a thread should be outside this util.

This will be run in the context of another spawned thread, so I'm not sure we should spawn another thread for it.
Let's discuss f2f.

Code quote:

tokio::task::spawn_blocking

crates/starknet_sierra_compile/src/compile.rs line 44 at r8 (raw file):

    // TODO(task_executor).
    tokio::task::spawn_blocking(move || starknet_sierra_compile(compilation_args, contract_class))
        .await?

What is returned in case of a panic? Does it catch it?
Please document.

Code quote:

.await?

crates/starknet_sierra_compile/src/compile_test.rs line 45 at r8 (raw file):

        entry_points_by_type,
        abi,
    };

Suggestion:

    let mut contract_class = contract_class_from_file(sierra_path);
    contract_class.sierra_program = sierra_program[..100].to_vec();
 

crates/starknet_sierra_compile/src/compile_test.rs line 53 at r8 (raw file):

    } else {
        panic!("Unexpected error.")
    }

Use assert_matches!

Code quote:

    if let CompilationUtilError::AllowedLibfuncsError(AllowedLibfuncsError::SierraProgramError) =
        result.unwrap_err()
    {
        return;
    } else {
        panic!("Unexpected error.")
    }

crates/starknet_sierra_compile/src/lib.rs line 1 at r8 (raw file):

//! An experimental attempt to compile Sierra into Casm by using cairo-lang as a lib .

Suggestion:

//! A lib for compiling Sierra into Casm.

crates/starknet_sierra_compile/src/test_utils.rs line 10 at r8 (raw file):

/// See: https://github.com/starkware-libs/cairo/blob/d0c0f175a8855242d8c6265c55d3f97f8dfdce40/crates/bin/starknet-sierra-compile/src/main.rs#L34-L43
/// Same as `ContractClass` - but ignores `abi` in deserialization.
/// Enables loading old contract classes.

Suggestion:

/// Same as `ContractClass` - but ignores unnecessary fields like `abi` in deserialization.
/// Enables loading old contract classes.

crates/starknet_sierra_compile/src/test_utils.rs line 12 at r8 (raw file):

/// Enables loading old contract classes.
#[derive(Deserialize)]
struct ContractClassIgnoreAbi {

Suggestion:

DeserializedContractClass

crates/starknet_sierra_compile/src/test_utils.rs line 17 at r8 (raw file):

    pub contract_class_version: String,
    pub entry_points_by_type: ContractEntryPoints,
    pub _abi: Option<serde_json::Value>,

I think you can just delete this.

Code quote:

pub _abi: Option<serde_json::Value>,

Reviewable status: 7 of 13 files reviewed, 10 unresolved discussions (waiting on @dafnamatsry and @giladchase)

crates/starknet_sierra_compile/src/compile.rs line 13 at r8 (raw file):

pub mod compile_test;

pub struct SierraToCasmCompliationArgs {

Done.

crates/starknet_sierra_compile/src/compile.rs line 15 at r8 (raw file):

Done.

crates/starknet_sierra_compile/src/compile.rs line 43 at r8 (raw file):

We use the thread here because this is our way to catch panics.

But I see what you are saying.
We can export the function compile_sierra_to_casm and mark (through documentation) as a function that might panic.

Let use discuss f2f.

crates/starknet_sierra_compile/src/compile.rs line 44 at r8 (raw file):

Done.
I think I was over-explicit.

crates/starknet_sierra_compile/src/compile_test.rs line 45 at r8 (raw file):

        entry_points_by_type,
        abi,
    };

Done.

crates/starknet_sierra_compile/src/compile_test.rs line 53 at r8 (raw file):

Done.

crates/starknet_sierra_compile/src/lib.rs line 1 at r8 (raw file):

//! An experimental attempt to compile Sierra into Casm by using cairo-lang as a lib .

Done.

crates/starknet_sierra_compile/src/test_utils.rs line 10 at r8 (raw file):

/// See: https://github.com/starkware-libs/cairo/blob/d0c0f175a8855242d8c6265c55d3f97f8dfdce40/crates/bin/starknet-sierra-compile/src/main.rs#L34-L43
/// Same as `ContractClass` - but ignores `abi` in deserialization.
/// Enables loading old contract classes.

Done.

crates/starknet_sierra_compile/src/test_utils.rs line 12 at r8 (raw file):

/// Enables loading old contract classes.
#[derive(Deserialize)]
struct ContractClassIgnoreAbi {

Done.

crates/starknet_sierra_compile/src/test_utils.rs line 17 at r8 (raw file):

Done.

Reviewable status: 7 of 13 files reviewed, 10 unresolved discussions (waiting on @dafnamatsry and @orizi)

crates/starknet_sierra_compile/src/compile.rs line 43 at r8 (raw file):

Adding on what dafna said: we'll soon limit all of our spawning to spawns from the executor, so if we spawn here we'll have to pass the executor as an argument into this API, which might be considered out-of-scope of what this crate should know about.

Reviewed 4 of 6 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: 12 of 13 files reviewed, 2 unresolved discussions (waiting on @giladchase and @orizi)

Reviewable status: 9 of 13 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry, @giladchase, and @orizi)

crates/starknet_sierra_compile/src/compile.rs line 43 at r8 (raw file):

Done.

Reviewable status: 9 of 13 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry, @giladchase, and @orizi)

crates/starknet_sierra_compile/src/compile.rs line 43 at r8 (raw file):

Now not using threads and tokio at all in this PR.

Reviewable status: 9 of 13 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry, @giladchase, and @orizi)

crates/starknet_sierra_compile/src/compile.rs line 44 at r8 (raw file):

No longer relevant.

Reviewed 4 of 4 files at r11, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @giladchase and @orizi)